Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In pulls, compute DiffIDs earlier, and also for v2s2 #2635

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Nov 18, 2024

This is primarily motivated by preparing for the zstd:chunked logic, but potentially useful separately:

  • Have the generic code provide the image config, and thus DiffIDs, at the start of the copy. Therefore we can start committing staged chunked layers immediately, we don’t have to wait for all of them to be staged. That might be a fairly significant performance benefit.
  • Support obtaining DiffIDs from v2s2 images as well. Right now that only happens in theoretical corner cases; we will want to enforce that for all images in the future.
  • (Some mostly-unrelated cleanups of handling of zero-length manifests.)

At this point absolutely untested.

Cc: @giuseppe , filing early to allow a preliminary review in case I have missed anything important.

@mtrmac mtrmac force-pushed the diffid-earlier-always branch 3 times, most recently from 9c6027e to 04ec0aa Compare November 21, 2024 20:13
@mtrmac
Copy link
Collaborator Author

mtrmac commented Nov 22, 2024

Now manually tested; I have confirmed that images.json and layers.json are unchanged by this, except for timestamps.

This is ready for review.

@mtrmac mtrmac marked this pull request as ready for review November 22, 2024 16:52
@mtrmac
Copy link
Collaborator Author

mtrmac commented Nov 22, 2024

… and, for the record, I have tested both the full PR and a variant without the last commit (testing the “fallback” image parsing implementation).

@mtrmac mtrmac force-pushed the diffid-earlier-always branch 2 times, most recently from e69286e to 85cee69 Compare November 25, 2024 20:40
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM
@giuseppe PTAL

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

If the value is set to a zero-byte value, use it, instead
of trying to look for a value again / elsewhere.

This should not make a difference in practice, a zero-length
manifest is invalid anyway; so it's just a conceptual cleanup
/ a microoptimization.

Signed-off-by: Miloslav Trmač <[email protected]>
Should not change behavior.

Signed-off-by: Miloslav Trmač <[email protected]>
Other parts of the code already assume that the value
is always valid, so don't treat an empty value specially.

Signed-off-by: Miloslav Trmač <[email protected]>
Always check for nil, not for len(0).

Ensure that PutManifest always sets it to non-nil,
so that valid call sequences (with an invalid empty manifest)
don't show up as hard-to-explain invariant violations.

Signed-off-by: Miloslav Trmač <[email protected]>
We will add one more user.

Should not change behavior.

Signed-off-by: Miloslav Trmač <[email protected]>
For now this is not really relevant, but we will want to enforce
the correctness of those values in the future.

Signed-off-by: Miloslav Trmač <[email protected]>
For now, this only adds the API, nothing actually benefits from
it yet.

Signed-off-by: Miloslav Trmač <[email protected]>
Record DiffIDs early, so that we can commit partially-pulled
layers immediately after staging them, and we don't have to wait
for PutManifest.

Signed-off-by: Miloslav Trmač <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants